Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement send_break support #700

Merged
merged 1 commit into from
Apr 13, 2024
Merged

Implement send_break support #700

merged 1 commit into from
Apr 13, 2024

Conversation

ithinuel
Copy link
Member

No description provided.

@jannic
Copy link
Member

jannic commented Oct 19, 2023

The failing test is the MSRV check also discussed in #698. It's fixed with #698, so no need to worry about it here.

jannic
jannic previously approved these changes Oct 19, 2023
rp2040-hal/src/uart/writer.rs Outdated Show resolved Hide resolved
rp2040-hal/src/uart/writer.rs Outdated Show resolved Hide resolved
rp2040-hal/src/uart/peripheral.rs Show resolved Hide resolved
@jannic jannic added the non-breaking change This pull requests contains a minor, not SemVer breaking, change label Oct 22, 2023
@jannic jannic added this to the 0.9.1 milestone Oct 22, 2023
@jannic
Copy link
Member

jannic commented Nov 5, 2023

While trying to write a good example for this function, I noticed a few things I don't like about this API:
First, this looks like valid code but is completely wrong:

     uart.write_full_blocking(data: b"before break\r\n");                         
     uart.send_break();                                            
     uart.write_full_blocking(data: b"after break\r\n");             

This compiles without a warning, but wouldn't ever clear the break bit.
By adding a #[must_use] annotation, this would at least trigger a warning:

warning: unused `rp2040_hal::uart::writer::Break` that must be used
   --> rp2040-hal/examples/uart.rs:101:5
    |
101 |     uart.send_break();
    |     ^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
101 |     let _ = uart.send_break();
    |     +++++++

But the suggestion is misleading (let _ = uart.send_break() would still be wrong), and doesn't provide any hint how to use it correctly.
If you are aware that you have to clear the break manually, and have to implement the delay yourself, the next attempt could be this:

    uart.write_full_blocking(b"before break\r\n");
    let brk = uart.send_break();
    let bit_duration: MicrosDurationU32 = baudrate.into_duration();
    let break_duration = bit_duration * (2 * 10); // at least two full frames of 10 bits
    delay.delay_us( break_duration.to_micros() );
    brk.clear();
    uart.write_full_blocking(b"after break\r\n");

But as far as I understand, it's still not correct. The break bit is specified as:
"If this bit is set to 1, a low-level is continually output on the UARTTXD output, after completing transmission of the current character. "
So the code has two issues:

  1. The break might start somewhere in the middle of the string "before break", because it will start after the currently transmitted byte, not flushing the TX fifo first. This is probably not what the user wants to do.
  2. The delay starts immediately, while the last frame before the break is still being transmitted. So at worst, the break might be one frame too short.
    To fix this, one needs to add a flush before starting the break.

All in all, this interface is easier to use incorrectly than correctly.

While less flexible, wouldn't something like fn send_break(delay: &mut DelayUs, duration_bits: u32) be much more usable? However, I'd hesitate to implement it like that before embedded_hal 1.0 is released, to avoid adding a dependency on embedded_hal 0.2.

For now, we could add two low-level functions set_break() / clear_break(), which just set and clear the break bit? This would be as difficult to use correctly as described above, but at least it wouldn't give the impression of being a high-level API.

@jannic jannic dismissed their stale review November 5, 2023 15:37

Realized that the API is too difficult to use correctly.

@jannic jannic modified the milestones: 0.9.1, 0.9.2 Nov 11, 2023
@ithinuel
Copy link
Member Author

I know it makes it harder to handle but would changing the type of the uart be acceptable? (via a wrapping newtype or a generic).

// with
fn break(self) -> Breaking<Self>;

let foo: Breaking<_> = serial.break();
// some wait logic
let serial = foo.unbreak();

@TKFRvisionOfficial
Copy link

TKFRvisionOfficial commented Apr 6, 2024

I'm not a fan of the @ithinuel solution to the issue, because I store my UART in a struct, this would break things cuz now we got a consuming function. Im of the opinion that @jannic thoughts about the misusage in regards to bypassing the must_use via "_=" is unfounded because ppl who do this probably haven't really understood what a uart break actually is yet. A issue I do get is the TX buffer not being cleared, which can be confusing to beginners that don't understand a 100% how the rp2040 uart works yet. In my small world proper docs would suffice but I get that rust is all about enforcing the correctness via the compiler. I don't really have an issue with @jannic less flexible API, it doesn't effect my use case, but their might be some edge case out there where this doesn't suffice. Adding the low level functions (maybe as unsafe) might fix this as well.

@9names
Copy link
Member

9names commented Apr 7, 2024

Rust is all about enforcing the correctness via the compiler.

Rust is all about enforcing memory safety, sometimes via the compiler, sometimes via the standard library.
We can not and should not solve all logic errors through the language and the typesystem.

Adding the low level functions (maybe as unsafe)

Putting low-level access behind "unsafe" is a terrible solution to this. This isn't a memory safety issue.
Having unsafe on a function like this means you unnecessarily opt out of some of the safety that safe Rust provides.

If you really want low-level interfaces to be super obvious (and therefore, not what you choose when looking for a high-level API) we have better practices for logically separating functions that live at different levels of the API.
Namespaces are a nice one (that would require more change to the existing code). But even just prefixing the function with the words "lowlevel_" makes it quite obvious that it's a low level function that you should take more care with:

serial.lowlevel_set_break();

Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not convinced that the lowlevel_-prefix is needed, it doesn't really hurt either. And instead of endlessly bikeshedding function naming, we should start providing a way to send a break using the HAL. → Approved.

@jannic jannic merged commit 840e90b into rp-rs:main Apr 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking change This pull requests contains a minor, not SemVer breaking, change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants